Job #1154 is now in scope, role is REV

This pull request #1154 is assigned to @vzurauskas/z, here is why; the budget is 15 minutes, see §4; please, read §27 and when you decide to accept the changes, inform @llorllale/z (the architect) right in this ticket; if you decide that this PR should not be accepted ever, also inform the architect; this blog post will help you understand what is expected from a code reviewer; there will be no monetary reward for this job

@vzurauskas ping?

Codecov Report

Merging #1154 into master will decrease coverage by 0.02%.
The diff coverage is 94.38%.

@@             Coverage Diff              @@
##             master    #1154      +/-   ##
============================================
- Coverage     89.35%   89.33%   -0.03%     
- Complexity     1588     1599      +11     
============================================
  Files           271      272       +1     
  Lines          3888     3899      +11     
  Branches        215      214       -1     
============================================
+ Hits           3474     3483       +9     
- Misses          379      381       +2     
  Partials         35       35

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5ff9e6d...b092754. Read the comment docs.

@victornoel Sorry, still fighting the ton of REV jobs in my agenda. A new option to limit them should be introduced soon, it should be better then.

@0crat refuse

@0crat refuse (here)

@vzurauskas The user @vzurauskas/z resigned from #1154, please stop working. Reason for job resignation: Order was cancelled

Tasks refusal is discouraged, see §6: -15 point(s) just awarded to @vzurauskas/z

This pull request #1154 is assigned to @scristalli/z, here is why; the budget is 15 minutes, see §4; please, read §27 and when you decide to accept the changes, inform @llorllale/z (the architect) right in this ticket; if you decide that this PR should not be accepted ever, also inform the architect; this blog post will help you understand what is expected from a code reviewer; there will be no monetary reward for this job

@scristalli I've pushed some change and answere your comments, thanks

@llorllale I approve the changes

@victornoel I come here from llorllale/cactoos-matchers#135

I'm not sure about the consequences of transferring equals, toString and hashcode from the envelopes to a "special" implementation of the interface.

From a usability perspective, it's the difference between

Existing way:

// we get equals, hashcode and toString for free
public final MyIterable<T> extends IterableEnvelope<T>{
    public MyIterable(final T ...elements) {
        super(args);
   }
}

And your way:

// we need IterableOf to get equals, hashcode and toString
public final MyIterable<T> extends IterableEnvelope<T>{
    public MyIterable(final T ...elements) {
        super(new IterableOf<>(args));
   }
}

Also, it's not clear why IterableOf should be the "special" class that provides those methods. They probably belong in another class? In which case, your way would involve additional decoration.

@llorllale First, let me remind you that there is a discussion in #947 that arrived to the conclusion that envelope should only delegates to wrapped object. Hence this PR. But I completely share the design decisions here, so let me try to explain them.

To me: one class = one responsibility.

An envelope, by its name, is something whose responsibility is to envelope. Actually I see it as a pattern, it doesn't implement behaviour, it helps implementing behaviour in other class by easing the delegation of implementation to another class. As in your second example. The problem with it, is that it's useless as an example, you could have used directly IterableOf itself. Now if you look at say, Cycled, in this PR: https://github.com/yegor256/cactoos/pull/1154/files#diff-7aac25700ee98a7068c93995c10fa5bc, it makes sense: you want to reuse some implementation, but you want to use it with specific constructor parameters.

Also, it's not clear why IterableOf should be the "special" class that provides those methods. They probably belong in another class? In which case, your way would involve additional decoration.

You are right, it doesn't. I could introduce a class named DefaultIterable, BasicIterable, CommonIterable or maybe it would be better to call it ScalarIterable, whose purpose is to implement all the contract of Iterable, based on a Scalar that returns a Iterator.

Then IterableOf would extend IterableEnvelope and delegate to ScalarIterable in order to take advantage of its behaviour's implementation.

We actually could also implement ArrayIterable which is an iterable based on an array, so it would simplify implementation and performance of the methods (behaviour). Then when other classes would need to delegate to one of the classes implementing the common behaviour, they can choose: do I delegate to ArrayIterable or do I delegate to ScalarIterable? And so on.

See there already you have two different implementation of the same behaviour: IterableEnvelope is only there to help ease the usage of one or the other in your decorator, without imposing one on you as it is currently the case (and was the initial problem from #947, kind of)

@llorllale oups, forgot to ping you in my previous answer, I edited it :)

@llorllale you will notice I did a mixup in my head between Text and Iterable, I will edit it, sorry about that :D

@llorllale it's done, sorry again for the confusion

@llorllale two more points:

1. At the very least, if you want to use inheritance to reuse behaviour, do not call the parent class an envelope, but call it AbstractIterable or AbstractCommonIterable.
2. by having a ScalarIterable for example as above instead of an AbstractIterable, is that you can compose multiple decorator while you can't with inheritance.

@llorllale can we get some conlusion on this and get it merged if it's ok :)

@victornoel I appreciate your insights but I'm afraid that discussion is not over yet. You decide - you can either modify this PR or close it for now.

@llorllale ok, what would you want me to change then so that this can be merged?

@llorllale/z everybody who has role REV is banned at #1154; I won't be able to assign anyone automatically; consider assigning someone manually (as in §19), or invite more people (as in §51), or remove the job from the scope (as in §14)

@llorllale/z everybody who has role REV is banned at #1154; I won't be able to assign anyone automatically; consider assigning someone manually (as in §19), or invite more people (as in §51), or remove the job from the scope (as in §14)

@llorllale/z everybody who has role REV is banned at #1154; I won't be able to assign anyone automatically; consider assigning someone manually (as in §19), or invite more people (as in §51), or remove the job from the scope (as in §14)

@llorllale/z everybody who has role REV is banned at #1154; I won't be able to assign anyone automatically; consider assigning someone manually (as in §19), or invite more people (as in §51), or remove the job from the scope (as in §14)

This pull request #1154 is assigned to @fabriciofx/z, here is why; the budget is 15 minutes, see §4; please, read §27 and when you decide to accept the changes, inform @paulodamaso/z (the architect) right in this ticket; if you decide that this PR should not be accepted ever, also inform the architect; this blog post will help you understand what is expected from a code reviewer; there will be no monetary reward for this job

@paulodamaso please, check it out why travis and appveyour fail. I cant' approve a PR that fails build.

@fabriciofx I've rebased on master for the build to pass and applied the change you asked for. It should be alright know

@fabriciofx it's done, thanks

@victornoel it seems fine to me. Congratulations for this nice PR! @paulodamaso can you merge it, please?

@rultor merge

@rultor merge

@paulodamaso OK, I'll try to merge now. You can check the progress of the merge here

@rultor merge

@paulodamaso Done! FYI, the full log is here (took me 15min)

@paulodamaso/z all QAs are on vacation. Please handle that.

The job #1154 is now out of scope

Payment to ARC for a closed pull request, as in §28: +10 point(s) just awarded to @paulodamaso/z